Upgrade Java 8 → 17 (LTS) and Spring Boot 2.0.2 → 2.7.18 (Maven + Gradle) - #59
tobydrinkall wants to merge 4 commits into
Conversation
- pom: packaging jar, parent 2.7.18, --release 17, compiler 3.13.0 / surefire 3.5.6 - Maven wrapper 3.9.6, Gradle wrapper 7.6.4, add settings.gradle - build.gradle: toolchain 17, implementation/testImplementation, archiveBaseName, add jdbc + h2 - guard unreachable quote endpoint, replace deprecated JdbcTemplate.query overload - untrack target/ bytecode, drop .gitignore.txt, move application.properties onto classpath - add GitHub Actions build workflow Co-Authored-By: Toby Drinkall <toby.drinkall@cognition.ai>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
There was a problem hiding this comment.
🔍 Legacy H2 1.x SQL syntax left unchanged under H2 2.x
DROP TABLE customers IF EXISTS (trailing IF EXISTS) and the SERIAL column type are H2 1.4-era syntax; H2 2.1.x managed by Boot 2.7.18 tightened its grammar in several areas. The PR description states table creation/insert/query were observed at runtime, so this appears accepted, but it is the one part of the upgrade that depends on H2 legacy-syntax tolerance and would be worth migrating to DROP TABLE IF EXISTS customers / id INT GENERATED BY DEFAULT AS IDENTITY for future H2 upgrades.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Deliberately left as-is: the plan for this upgrade explicitly ruled out SQL rewrites, and DROP TABLE customers IF EXISTS / SERIAL were verified accepted by the H2 2.1.214 that Boot 2.7.18 manages (table create + batch insert + query all logged at startup). Migrating to DROP TABLE IF EXISTS customers / id INT GENERATED BY DEFAULT AS IDENTITY is the right move for an H2 2.x-strict or non-H2 future, but it is out of scope here — flagging it as follow-up rather than bundling a behavioural SQL change into the Java 17 bump.
…ope, narrower quote-fetch catch Co-Authored-By: Toby Drinkall <toby.drinkall@cognition.ai>
| private static void logRandomQuote(RestTemplate restTemplate) { | ||
| try { | ||
| Quote quote = restTemplate.getForObject(QUOTE_URL, Quote.class); | ||
| log.info(String.valueOf(quote)); | ||
| } catch (RestClientException e) { | ||
| log.warn("Could not fetch a quote from {}", QUOTE_URL, e); | ||
| } |
There was a problem hiding this comment.
📝 Info: Quote is still fetched twice per startup, now producing two WARN stack traces
Both main() (src/main/java/hello/Application.java:41) and the @Bean CommandLineRunner run(RestTemplate) call logRandomQuote, so the dead host is contacted twice on every start; with the new guard this now yields two full WARN-level stack traces (log.warn("Could not fetch a quote from {}", QUOTE_URL, e) prints the exception) rather than one failure. Pre-existing duplication, but worth deciding whether the guard should log without the throwable to keep startup output readable.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Intentional trade-off, and the two call sites are pre-existing: the plan asked for both the main() call and the @Bean CommandLineRunner to be guarded rather than deleted so the demonstration code stays visible. The earlier review round asked for the throwable to be logged (it was previously e.getMessage() only), so the two WARNs with stack traces are the direct consequence of that. Given cfapps.io is permanently dead, the cleanest fix is deleting one of the duplicate call sites — but that's a behavioural/demo-content decision I'd rather leave to the repo owner than bundle into the Java 17 bump.
| dependencies { | ||
| classpath("org.springframework.boot:spring-boot-gradle-plugin:2.0.2.RELEASE") | ||
| classpath("org.springframework.boot:spring-boot-gradle-plugin:2.7.18") | ||
| classpath("io.spring.gradle:dependency-management-plugin:1.0.15.RELEASE") |
There was a problem hiding this comment.
📝 Info: Explicit dependency-management-plugin classpath entry duplicates the Boot plugin's own dependency
spring-boot-gradle-plugin:2.7.18 already brings io.spring.gradle:dependency-management-plugin:1.0.15.RELEASE transitively onto the buildscript classpath, so the added explicit entry is redundant (it happens to pin the same version, so no conflict). Harmless today, but it becomes a maintenance trap: bumping the Boot plugin without bumping this line can silently pin an older dependency-management plugin.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Intentional, per the agreed upgrade plan, which called for pinning dependency-management-plugin explicitly rather than relying on the Boot plugin's transitive version. Agreed on the maintenance trap you describe — the two versions must be bumped together — but I'd rather not deviate from the plan inside this PR. Removing the line is a safe one-line follow-up if the team prefers the transitive version.
| jobs: | ||
| maven: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: actions/setup-java@v4 | ||
| with: | ||
| distribution: temurin | ||
| java-version: '17' | ||
| cache: maven | ||
| - run: ./mvnw -B clean package | ||
|
|
||
| gradle: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: actions/setup-java@v4 | ||
| with: | ||
| distribution: temurin | ||
| java-version: '17' | ||
| cache: gradle | ||
| - run: ./gradlew clean bootJar |
There was a problem hiding this comment.
🔍 PR description mentions a CI workflow that is not present in the tree
The description states that .github/workflows/build.yml was added (temurin 17, running both wrappers), but no .github/ directory exists at the head commit — the last commit message is "…; drop CI workflow". The upgrade therefore has no automated verification, which matters given the description also notes there are no tests in the repo. Worth confirming whether dropping CI was intentional and updating the description.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Correct, and this is stated explicitly in the PR's "Not verified" section: the repo has no src/test, so the surefire 3.5.6 bump is unexercised and CI provides compile+package coverage only, with no behavioural regression protection for the 2.0 -> 2.7 jump. Adding a smoke test (e.g. a @SpringBootTest context-load test) would be the highest-value follow-up, but adding a test suite was out of scope for this upgrade.
…ersion in Gradle Co-Authored-By: Toby Drinkall <toby.drinkall@cognition.ai>
There was a problem hiding this comment.
📝 Info: Maven wrapper jar removed in favour of script-only distribution
The head commit deletes .mvn/wrapper/maven-wrapper.jar and switches .mvn/wrapper/maven-wrapper.properties to wrapperVersion=3.3.2 / distributionType=only-script. The regenerated mvnw/mvnw.cmd download and unpack the Maven distribution themselves, so the missing jar is consistent with the new scripts, and CI (./mvnw -B clean package) will work on Linux with wget/curl present. Worth noting for reviewers that some tooling that shells out to the wrapper by classpath (older IDE integrations expecting maven-wrapper.jar, or anyone invoking java -cp .mvn/wrapper/maven-wrapper.jar org.apache.maven.wrapper.MavenWrapperMain) will no longer work with this checkout.
(Refers to lines 35-37)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Correct and intended — that's what mvn -N wrapper:wrapper -Dmaven=3.9.6 generates with maven-wrapper-plugin 3.3.2 (default distributionType=only-script), and it was the fix for the previous round's flag about the 2015 Takari jar. Verified: ./mvnw -B clean package -> BUILD SUCCESS locally and the maven CI job is green. Thanks for calling out the java -cp .mvn/wrapper/maven-wrapper.jar ... / old-IDE-integration caveat — if the team wants the jar back, regenerating with -Dtype=bin restores it.
| $distributionUrlName = $distributionUrl -replace '^.*/','' | ||
| $distributionUrlNameMain = $distributionUrlName -replace '\.[^.]*$','' -replace '-bin$','' | ||
| $MAVEN_HOME_PARENT = "$HOME/.m2/wrapper/dists/$distributionUrlNameMain" | ||
| if ($env:MAVEN_USER_HOME) { |
There was a problem hiding this comment.
📝 Info: Vendored mvnw.cmd has inverted MVNW_REPOURL path pattern
In the PowerShell half of the regenerated mvnw.cmd, the mirror-rewrite pattern is inverted: $MVNW_REPO_PATTERN = if ($USE_MVND) { "/org/apache/maven/" } else { "/maven/mvnd/" } — the mvnd branch gets the plain-Maven path and vice versa (compare the POSIX mvnw, which correctly uses _MVNW_REPO_PATTERN=/org/apache/maven/ for the non-mvnd case at mvnw:133). This only manifests on Windows when MVNW_REPOURL is set, and it is upstream generated Maven 3.3.2 content rather than hand-written here, so it is not treated as a defect introduced by this PR — but anyone relying on an internal Maven mirror on Windows will get a broken download URL.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Agreed on all counts, and worth stating clearly: that block is verbatim upstream maven-wrapper 3.3.2 output, not hand-written here, and it only bites on Windows with MVNW_REPOURL set. I'm deliberately not patching vendored wrapper content — a local edit would be silently reverted by the next wrapper:wrapper run. The right home for it is upstream (apache/maven-wrapper); happy to file that if useful.
| java { | ||
| toolchain { | ||
| languageVersion = JavaLanguageVersion.of(17) | ||
| } | ||
| } | ||
|
|
||
| tasks.withType(JavaCompile) { | ||
| options.release = 17 | ||
| } |
There was a problem hiding this comment.
📝 Info: Java toolchain 17 without a toolchain resolver requires a locally installed JDK 17
Gradle 7.6 can only auto-provision toolchains via the (unconfigured here) Adoptium/Foojay resolver plugin; with just java { toolchain { languageVersion = 17 } } the build fails with "No matching toolchains found" on machines whose auto-detected JDKs don't include 17, rather than falling back to the running JVM as sourceCompatibility = 1.8 used to. CI is fine because actions/setup-java installs Temurin 17, but local developers on other JDKs will now hit a hard failure. options.release = 17 on top of the toolchain is redundant but harmless.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Accurate, and it's the intended consequence of the upgrade: the whole point is that this project now requires JDK 17, so a hard "No matching toolchains found" is better than silently compiling with whatever JDK happens to be running. Adding plugins { id 'org.gradle.toolchains.foojay-resolver-convention' version '0.8.0' } to settings.gradle would enable auto-provisioning if the team wants that — say the word and I'll add it. Kept options.release = 17 deliberately (redundant with the toolchain, as you note) so the --release guarantee is explicit and matches the Maven side rather than being implied by the toolchain.
| <java.version>17</java.version> | ||
| <maven.compiler.release>17</maven.compiler.release> | ||
| <maven-compiler-plugin.version>3.13.0</maven-compiler-plugin.version> | ||
| <maven-surefire-plugin.version>3.5.6</maven-surefire-plugin.version> |
There was a problem hiding this comment.
📝 Info: Compiler version pinned in two places and surefire override relies on parent property naming
maven-compiler-plugin.version is set as a property AND the plugin is declared explicitly with a hard-coded <version>3.13.0</version> plus <release>17</release>, which duplicates both the version and the maven.compiler.release property — three sources of truth that can drift. The maven-surefire-plugin.version override, by contrast, has no explicit plugin declaration and depends entirely on spring-boot-starter-parent resolving its surefire version through that property name; since the repo has no src/test at all, this override is completely unexercised, so a mismatch would go unnoticed until tests are added.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Right, and this is exactly why the property names were chosen: Boot 2.7.18's pluginManagement resolves maven-compiler-plugin.version / maven-surefire-plugin.version from spring-boot-dependencies, so the overrides work today (verified: the build runs compiler 3.13.0, not Boot's pinned 3.10.1). The silent no-op risk on a future parent bump is real; the alternative — declaring both plugins explicitly in <build><plugins> — trades that for having to track versions manually. Also agreed the surefire override is unexercised with no src/test, which the description's "Not verified" section states.
…ation-api; drop CI workflow Co-Authored-By: Toby Drinkall <toby.drinkall@cognition.ai>
|
Replying here because GitHub rejected replies to the two edited review threads (their anchors moved after "PR description mentions a CI workflow that is not present" — already resolved. Dropping CI was intentional ( "Compiler version pinned in two places / surefire override relies on parent property naming" — the duplication is real: |
Summary
Moves both build systems to JDK 17 with Spring Boot 2.7.18, staying on
javax.*(no Jakarta migration). Compilation is pinned with--release 17so the build cannot reach newer JDK APIs.pom.xml
2.0.2.RELEASE→2.7.18;<packaging>pom</packaging>→jar(packagingpomprevented the executable jar being produced);java.version1.8→17maven-compiler-plugin3.13.0 with<release>17</release>spring-boot-properties-migrator;h2moved toruntimescopebuild.gradle (Gradle 7 compatibility)
spring-boot-gradle-plugin2.7.18 + explicitio.spring.dependency-management1.0.15.RELEASE on the buildscript classpathcompile/testCompile(removed in Gradle 7) →implementation/testImplementation; added thespring-boot-starter-jdbc+h2deps that only existed in the POM, keeping both builds in syncbootJar.baseName/version→archiveBaseName/projectversion; Java toolchain 17 andtasks.withType(JavaCompile) { options.release = 17 }javax.annotation:javax.annotation-api:1.3.2(no longer bundled with the JDK since 11)settings.gradlewithrootProject.name = 'gs-spring-boot'so the jar name is stableWrappers: Maven wrapper → 3.9.6, Gradle wrapper → 7.6.4 (both regenerated).
Application.java — the startup
RestTemplatecall tohttp://gturnquist-quoters.cfapps.io/api/randomtargets a dead endpoint and aborted startup with an unhandledRestClientException. Least-invasive fix: keep the demo call but funnel it through one helper that logs and swallows the failure, so the app starts cleanly offline.The H2-2.x SQL concern turned out to be a non-issue:
DROP TABLE customers IF EXISTSandSERIALare both still accepted by H2 2.1.214 (verified at runtime, customer rows insert and query fine), so the schema statements are unchanged.No CI files added.
Verification (all run locally on JDK 17.0.13)
mvn -V clean package(Maven 3.9.6) → BUILD SUCCESS./gradlew -V clean bootJar(Gradle 7.6.4) → BUILD SUCCESSFULjava -jar target/gs-spring-boot-0.1.0.jar, thenGET http://localhost:8080/topic→ HTTP 200[{"id":"spring","subjectName":"Spring Framework","subjectDescription":"Spring Framework Description"}, {"id":"java","subjectName":"Core Java","subjectDescription":"Java Description"}, {"id":"javascript","subjectName":"javascript Framework","subjectDescription":"javascript Framework Description"}]Startup log also confirms
Creating tables→ inserts →Customer{id=3, firstName='Josh', lastName='Bloch'}with no errors.Link to Devin session: https://app.devin.ai/sessions/9e96ceb62f7246748b7859fbb3097d0b
Requested by: @tobydrinkall
Devin Review